-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix broken CI and improve it #57
Conversation
New linting job almost verbatim copied from pfctl-rs
We have decided in another library project that it does not make much sense to audit library-only crates. It creates more needless churn than it helps.
Workflow copied from pfctl-rs
Copied from nftnl-rs. Also checks minimal versions now. Also checks that docs build cleanly
Good practice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faern)
.github/workflows/build-and-test.yml
line 24 at r1 (raw file):
# Keep MSRV in sync with rust-version in Cargo.toml rust: [stable, beta, nightly, 1.64.0] runs-on: macos-latest
We could probably throw *-apple-ios
targets into the matrix here, or at least aarch64-apple-ios
.
.github/workflows/linting.yml
line 14 at r1 (raw file):
jobs: clippy-linting: runs-on: macos-latest
We could probably throw *-apple-ios
targets into the matrix here, or at least aarch64-apple-ios
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)
.github/workflows/build-and-test.yml
line 24 at r1 (raw file):
Previously, dlon (David Lönnhager) wrote…
We could probably throw
*-apple-ios
targets into the matrix here, or at leastaarch64-apple-ios
.
This PR is still strictly an upgrade even if we don't do that. So unless you see a lot of benefit in doing so, I'd prefer to get this merged without that, and then anyone can tinker with improving it in that way later if they feel like it. I don't have the time to work too much on this right now. I mostly want to just fix the CI.
My understanding is that your feedback is a suggestion for an unrelated improvement, rather than an issue with the current state of the PR?
The initiative for this comes from the current CI being broken. See https://github.com/mullvad/system-configuration-rs/actions/runs/10504169497/job/29099041677
This is sort of a follow up to all the CI improvements we did in
pfctl-rs
andnftnl-rs
a few months back. This repository also needed some love when it comes to CI. Good we have great, pretty much just copy-paste workflows from other crates.I also added the
permissions: {}
thing. Something I just today started working on in our main repo also: mullvad/mullvadvpn-app#6660This change is